Skip to content

bpo-41735: Fix thread locks in zlib module may go wrong in rare case.#22126

Merged
ambv merged 2 commits into
masterfrom
unknown repository
Apr 27, 2021
Merged

bpo-41735: Fix thread locks in zlib module may go wrong in rare case.#22126
ambv merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Sep 7, 2020

Copy link
Copy Markdown

Setting `next_in` before acquiring the thread lock may mix up compress/decompress state in other threads.
@iritkatriel

Copy link
Copy Markdown
Member

Can you add a unit test that fails without the fix and passes with it?

@ghost

ghost commented Sep 7, 2020

Copy link
Copy Markdown
Author

Can you add a unit test that fails without the fix and passes with it?

Run this code:

import zlib
import threading

with open('txt.tar', 'rb') as f:
    b = f.read()
    b = b[:100*1024*1024]
print('read', len(b))

cobj = zlib.compressobj()

def f(name, cobj, b):
    print(name)
    dat = cobj.compress(b)
    print(name, len(dat))

thread1 = threading.Thread(target=f, args=('T1', cobj, b))
thread2 = threading.Thread(target=f, args=('T2', cobj, b))
thread1.start()
thread2.start()

print('ok')

Before the patch, output:

read 104857600
T1
T2
ok
T1 57142315

It seems thread2 did not finish the compressing.

After the patch, output:

read 104857600
T1
T2
ok
T1 57165411
T2 57166659

@ghost

ghost commented Sep 7, 2020

Copy link
Copy Markdown
Author

Can you add a unit test that fails without the fix and passes with it?

Seems a little troublesome. If the data is large, it may be time-consuming. If the data is small, it may not trigger the bug.

Above code can reliably reproduce the bug.

Comment thread Modules/zlibmodule.c
@ambv ambv merged commit 93f4118 into python:master Apr 27, 2021
@bedevere-bot

Copy link
Copy Markdown

@ambv: Please replace # with GH- in the commit message next time. Thanks!

@ghost ghost deleted the thread_lock branch April 27, 2021 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants